Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Index buffers reverted to dirty flag instead of timestamps #2927

Merged
merged 20 commits into from
Oct 16, 2024
Merged

Index buffers reverted to dirty flag instead of timestamps #2927

merged 20 commits into from
Oct 16, 2024
+27 −119

Conversation

alexcristici
Copy link
Collaborator

@alexcristici alexcristici commented Oct 11, 2024 •
edited
Loading

The index buffers were recreated for each drawable even though 2 or more drawables used the same index buffer.

We need to recreate index buffers only when the buffer data is dirty.
New index buffer will be shared through the indexes variable for all drawables that are using it.

This change also fixes the Android emulator crash.

I also removed some unused variables: indexBuffer, attributeBuffers and instanceBuffers.

@alexcristici alexcristici self-assigned this Oct 11, 2024
@alexcristici alexcristici linked an issue Oct 11, 2024 that may be closed by this pull request
Unused index buffers #2760
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 11, 2024 •
edited
Loading

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.0% -2.43Ki  -0.1% -16.0Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2927-compared-to-main.txt

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 11, 2024 •
edited
Loading

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.0% -21.4Ki  -0.0% -5.07Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2927-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +28% +32.6Mi  +426% +25.4Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2927-compared-to-legacy.txt

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 11, 2024 •
edited
Loading

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0023         -0.0019             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2927-compared-to-main.txt

@louwers
Copy link
Collaborator

louwers commented Oct 11, 2024

Could you provide some more context in the PR description?

@alexcristici
Copy link
Collaborator Author

Could you provide some more context in the PR description?

Sure, not ready yet, I thought I created as a draft. I will change now.

@alexcristici alexcristici marked this pull request as draft October 11, 2024 14:22
@alexcristici alexcristici changed the title Buffer revert to dirty flag Index buffers reverted to dirty flag instead of modified time Oct 11, 2024
@alexcristici alexcristici changed the title Index buffers reverted to dirty flag instead of modified time Index buffers reverted to dirty flag instead of timestamps Oct 11, 2024
@alexcristici alexcristici marked this pull request as ready for review October 11, 2024 19:58
@alexcristici
Copy link
Collaborator Author

Ready for review.

@sjg-wdw
Copy link
Collaborator

sjg-wdw commented Oct 15, 2024

@TimSylvester cool with you?

mwilsnd
mwilsnd approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@mwilsnd mwilsnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to be resolved, nice find!

@alexcristici
Copy link
Collaborator Author

Removed workaround for Android emulator crash: #2858.

@louwers louwers merged commit e937b0d into maplibre:main Oct 16, 2024
39 checks passed
@alexcristici alexcristici deleted the buffer-revert-to-dirty-flag branch October 16, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimSylvester TimSylvester TimSylvester approved these changes

@mwilsnd mwilsnd mwilsnd approved these changes

@louwers louwers louwers approved these changes

@adrian-cojocaru adrian-cojocaru Awaiting requested review from adrian-cojocaru

@alasram alasram Awaiting requested review from alasram

@cgalvan cgalvan Awaiting requested review from cgalvan

@sjg-wdw sjg-wdw Awaiting requested review from sjg-wdw

Assignees

@alexcristici alexcristici

Labels
None yet
Projects
MapLibre Native Project
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unused index buffers
5 participants
@alexcristici @louwers @sjg-wdw @mwilsnd @TimSylvester